LPS-119066 [Page Audit] Display the Google PageSpeed issues sections with total count #1005
Conversation
|
CI is automatically triggering the following test suites:
|
liferay-continuous-integration
added
ci:test:sf - pending
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPS-119066.issues-list 1 Successful Jobs:For more details click here. |
|
Jenkins Build:test-portal-source-format#5718 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#1005 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - dgarciasarai > liferay-frontend - PR#1005 - 2021-04-28[03:42:34] Testray Importer:publish-testray-report#507 |
✔️ ci:test:stable - 9 out of 9 jobs passed✔️ ci:test:relevant - 24 out of 25 jobs passed in 2 hours 15 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 57e7fbc138485535fb62a9c888f6b158b3452e82 ci:test:stable - 9 out of 9 jobs PASSED9 Successful Jobs:
ci:test:relevant - 23 out of 25 jobs PASSED2 Failed Jobs:23 Successful Jobs:
For more details click here.This pull contains no unique failures.Failures in common with acceptance upstream results at 57e7fbc:
Test bundle downloads:
|
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#7958 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#1005 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - dgarciasarai > liferay-frontend - PR#1005 - 2021-04-28[03:45:08] Testray Importer:publish-testray-report#511 |
|
Hey @liferay-frontend, can you review these changes and give me feedback? Thanks! 😄 |
| * details. | ||
| */ | ||
|
|
||
| window.Liferay.Util.sub = function (string = '', data) { |
There was a problem hiding this comment.
So we have similar logic now in:
modules/dxp/apps/segments/segments-experiment-web/jest-setup.config.jsmodules/dxp/apps/analytics/analytics-reports-web/jest-setup.config.js
I'm making an issue to remove this duplication by baking a reasonable substitute into our default mock environment. Will come back and update the link here once I've done that.
There was a problem hiding this comment.
Here's the link: liferay/liferay-frontend-projects#519
There was a problem hiding this comment.
That config is used to substitute the x in the message keys to check the full text in the tests. It will be awesome to not be required to add it each module. I saw that we are the only team using it in our tests 🤔 layout-reports-web, segments-experiment-web and analytics-reports-web.
| } | ||
|
|
||
| const dataCopy = [...data]; | ||
| const max = REGEX_SUB.exec(string).length; |
There was a problem hiding this comment.
Mini-nit: max → maximum (per guidelines, abbreviations and all that)
Non-blocking feedback though, especially because this file might go away based on liferay/liferay-frontend-projects#519
There was a problem hiding this comment.
Thanks for the nit. Hopefully, this file will be removed soon and provide it by default in our tests 🍀
| const max = REGEX_SUB.exec(string).length; | ||
| let replacedValues = 0; | ||
|
|
||
| const replacestring = string.replace |
There was a problem hiding this comment.
Another non-blocking mini-nit: replacestring → replaceString
There was a problem hiding this comment.
Also here...Hopefully, this file will be removed soon and provide it by default in our tests 🍀
| "jest": { | ||
| "setupFiles": [ | ||
| "<rootDir>/jest-setup.config.js" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
(Outside the scope of this PR, but this is another thing that is more or less repeated verbatim in about 10 places in liferay-portal now. Seems like the kind of thing we might be able to abstract away... Just throwing that out there in case anybody reading this feels like writing up a ticket for it — I'm not totally convinced it's a good idea, so I'm not going to right now).
| .dropdown-menu__languages { | ||
| max-height: 295px; | ||
| } |
There was a problem hiding this comment.
Just referencing prior discussion about this pattern from your other pull, in case anybody wants to look.
|
|
||
| useEffect(() => { | ||
| if (loading && !error) { | ||
| const x = Date.now(); |
There was a problem hiding this comment.
This should be initial or something descriptive, not x.
But seeing as you have a green CI run, I'd just loop back and do it next time you touch this file, if you do.
There was a problem hiding this comment.
Thanks! I'll change it and push force 👍
| const interval = setInterval(() => { | ||
| const time = (Date.now() - x) / 1000; | ||
| const progress = | ||
| -200 * (1 / (1 + Math.pow(Math.E, time / 4)) - 0.5); |
There was a problem hiding this comment.
I didn't even know there was a Math.E constant. I'm not going to pretend to understand this, so...
There was a problem hiding this comment.
This is a calculation provided by @4lejandrito to calculate from 0 to ~100 (never gets to 100) and this is used to update the progress bar value because when we call Google PageSpeed endpoint we don't have any information about the progress so... Sorry? 😬
There was a problem hiding this comment.
It's a sigmoid function:
- Scaled to go from [0, 100). It goes up smoothly and never gets to 100.
- It's adjusted so that it gets close to 100 in around 15 seconds (the time it normally takes to complete the request to google pagespeed).
We need to agree on a better timing and I will send that in a follow up.
There was a problem hiding this comment.
I will also make the parameters (15 seconds and 100) more explicit by using variables.
| export function _getFormDataRequest(body, prefix, formData = new FormData()) { | ||
| Object.entries(body).forEach(([key, value]) => { | ||
| formData.append(`${prefix}${key}`, value); | ||
| }); | ||
|
|
||
| return formData; | ||
| } | ||
|
|
||
| /** | ||
| * Wrapper to `fetch` function throwing an error when `error` is present in the response | ||
| */ | ||
| function _fetchWithError(url, options = {}) { |
There was a problem hiding this comment.
Seems that:
- We don't need to export
_getFormDataRequest(it is only used internally) - We should never export a function with a leading underscore anyway
- We don't need to mark these functions as "private" by using an underscore
ie. I think we can delete the underscores, and get rid of the export
Again, because you have a clean CI run, feel free to forward if you want and just fold these changes in to the next time you touch this file.
There was a problem hiding this comment.
Ok! Sure! I'll update it and push force 👍
There was a problem hiding this comment.
No deal-breakers in here. If you want to forward and make the most of that green CI run, feel free to do so @dgarciasarai, and roll whatever follow-ups you have into your next change.
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#4258 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#1005 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - dgarciasarai > liferay-frontend - PR#1005 - 2021-04-29[04:06:23] Testray Importer:publish-testray-report#484 |
|
Should we restart the CI? 🤔 |
|
ci:forward |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
|
Skipping previously passed test suites: |
✔️ ci:test:stable - 9 out of 9 jobs passed❌ ci:test:relevant - 22 out of 25 jobs passed in 2 hours 22 minutesClick here for more details.This pull is eligible for reevaluation. When this upstream build has completed, using the following CI command will compare this pull request result against a more recent upstream result: ci:reevaluate:1177401_7632 Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: f8aa3697ce0df564a38c8fae2516e86001402ed2 ci:test:stable - 9 out of 9 jobs PASSED9 Successful Jobs:
ci:test:relevant - 22 out of 25 jobs PASSED3 Failed Jobs:22 Successful Jobs:
For more details click here.Failures unique to this pull:
Test bundle downloads:
|
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#7632 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#1005 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - dgarciasarai > liferay-frontend - PR#1005 - 2021-04-29[09:12:36] Testray Importer:publish-testray-report#496 |
|
ci:reevaluate:1177401_7632 |
|
CI is reevaluating the build with build ID: |
|
Previous ✔️ ci:test:stable - 9 out of 9 jobs passed✔️ ci:test:relevant - 23 out of 25 jobs passed in 2 hours 24 minutesClick here for more details.Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 318146d8babd408ff5f16b93b0e04428cbf3241f ci:test:stable - 9 out of 9 jobs PASSED9 Successful Jobs:
ci:test:relevant - 22 out of 25 jobs PASSED3 Failed Jobs:22 Successful Jobs:
For more details click here.This pull contains no unique failures.Failures in common with acceptance upstream results at 318146d:
|
|
ci:forward |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
|
Skipping previously passed test suites: |
|
All required test suite(s) passed. |
|
Pull request has been successfully forwarded to brianchandotcom#101288 |
Amazing. I don't think I've ever seen this work before...
|
Description
Page Audit allows to make a quick check of that compliance and get warnings and tips to improve each one of the categories. Once the API Key is enabled, the Page Audit panel will show the detail of the issues in two sections: Accessibility and SEO.
Within this pull request
Backend part already reviewed 😉
layoutReportsIssuesURLin each canonicalURL (backend stuff)Screenshot